Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[REP-2015] ROS 2 DDS Security PKCS#11 Support #375

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Mario-DL
Copy link

This PR introduces a new REP-2015 proposal based on ros2/design#332 concerning the ROS2 DDS Security PKCS#11 Support.

@Mario-DL Mario-DL changed the title REP-2015: ROS2 DDS Security PKCS#11 Support [REP-2015] ROS2 DDS Security PKCS#11 Support Mar 31, 2023
Copy link
Contributor

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the REP, I have a couple of comments.

Then, the RMW can be aware of the file extension and set the security property accordingly.
Taking the private key in the authentication plugin as an example:

#. The RMW will look for a file with name ``key.pem`` or ``key.p11`` in the enclave.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that possible that both files exist at the same time? if that is, pcks11 scheme prevails to file?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but only if the RMW supports it.

Some check can be done at this point, e.g., assert that the file contains a valid URI (i.e., starts with ``pkcs11:``).
#. Otherwise, if there is a ``key.pem`` file, it keeps the current behavior, and sets the value of the property to the path of the file.

With this proposal, current RMW implementations do not need to be updated if no support for PKCS#11 URIs is planned.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity, which rmw implementation actually supports pkcs11 scheme? especially tier I implementations. it would be probably nice to have all tier I implementations support this pkcs11 scheme for consistent user application behavior.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK, both Connext and Fast DDS support pkcs11. I'm not sure about Cyclone DDS

@MiguelCompany
Copy link

@fujitatomoya @clalancette Any chance this could be in soon? It is currently being discussed on the Security WG.

@ros-discourse
Copy link

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/ros-2-tsc-meeting-minutes-4-20-2023/31087/1

@ros-discourse
Copy link

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/announcing-rep-2015-ros-2-dds-security-pkcs-11-support/31357/1

@kscottz
Copy link
Contributor

kscottz commented May 11, 2023

Please stylize the name as "ROS 2" not "ROS2" as mentioned in the ROS style guide.

@Mario-DL Mario-DL changed the title [REP-2015] ROS2 DDS Security PKCS#11 Support [REP-2015] ROS 2 DDS Security PKCS#11 Support May 12, 2023
rep-2015.rst Outdated Show resolved Hide resolved
Signed-off-by: Miguel Company <[email protected]>

Co-authored-by: Michael Orlov <[email protected]>
Copy link

@MichaelOrlov MichaelOrlov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the abstract of this REP-2015

The DDS-Security specification [1] defines the use of Hardware Security Modules (HSM) and PKCS#11 URIs as an alternative to private keys and certificates stored in the file system.

However, the final proposal in this REP-2015 is to save PKCS#11 URIs in files.
It looks like it defeat the purpose of the PKCS#11 URIs.
Why do we need PKCS#11 URIs in this case? How are they will be different in terms of the vulnerability risks from pem files if they will be stored on the same files system and in the same folder?
It looks like the PKCS#11 URIs will have the same risks to be stollen as pem files.

Although, I might be wrong about PKCS#11 URIs since I don't have expertise in this area. Please correct me if I am wrong.

@MiguelCompany
Copy link

MiguelCompany commented Jan 30, 2024

The main difference is that the certificates and, more importantly, their private keys will be stored in a hardware security module (HSM), i.e. an external device.
Operations with the private keys will be executed inside the HSM, so they will never be in the computer's memory.

@MichaelOrlov
Copy link

@MiguelCompany Thanks for the explanation. Now it has become clear and does make sense.

Comment on lines +20 to +22
The DDS-Security specification [1] requires the security documents (private keys, certificates, governance and permissions) to be provided to the DDS implementation within the participant's ``properties``.
It defines a set of reserved properties, that must hold the URI of the corresponding document.
However, it allows different URI schemes to be used.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(aside from DDS specification)
as ROS 2 or RMW, i think it would be better to support this feature in general.
in robotics and robot application running on teleoperation or controller devices, those security data would be protected by hardware like TPM(Trusted Platform Module) and Hardware Security Module(HSM).
i think this could be one of the major motivation here.

@clalancette
Copy link
Contributor

I think overall the addition of this feature to SROS 2 makes sense. I have two ovearching comments here:

  1. While I know that this functionality is coming from the DDS side of things, I think this REP is too DDS-specific. It is fine to reference the DDS Security specification as an inspiration, but I think the rest of it should be DDS-agnostic. In particular, how would a non-DDS RMW implement this?
  2. Using the file extension as the way to distinguish between a file and a URI isn't great, in my opinion. I'd much rather see us explicitly pass some kind of metadata (an enum or something) through the layers so that it is clear what kind of data we are passing.

@ros-discourse
Copy link

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/ros-2-meeting-minutes-2024-02-15/36221/1

@MiguelCompany
Copy link

  1. I think this REP is too DDS-specific

Yes and no.
Regarding PKCS#11 URIs, they are defined in RFC 7512 so I would not consider them DDS-specific.

The original SROS 2 design article by @kyrofa is DDS-specific.

There has never been a REP on SROS 2, we opened this one because we were told to here.
Perhaps the original authors of the design documents @kyrofa, @ruffsl, and @mikaelarguedas could come up with a proper REP on ROS 2 security.

Regarding how would a non-DDS RMW implement this, the interfaces are designed so that all the RMW receives is a string with the security enclave. How that string translates into specific security artifacts is not part of the RMW interface.

I think this also answers point 2. I should also point out that this was discussed in the security WG, and it was the best solution we found meeting the following requirements:

  • The tooling (i.e. ros2 security ... CLI) should not break. Meaning old files should stay as they are
  • If a DDS RMW implementation does not have support for PKCS#11 URIs, it should have the direct file access option available

The contents of this REP might need a refactor (perhaps even a full rework to convert it into a proper ROS 2 security REP), but I don't think that should block the corresponding implementation PRs:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants